Made LaunchConfiguration asynchronous at the language level
#1019
Conversation
Lehonti
changed the title
LaunchConfiguration asynchronousLaunchConfiguration asynchronous at the language level
There was a problem hiding this comment.
I think this is the right approach. The API actually used to return a bool back in the GTK3 days when the dialog was blocking, but then these event handlers were introduced in the GTK4 port (ef0466d) which happened before we got the synchronization contexts and async methods working properly
| /// The localizer for the effect add-in. This is used to fetch translations for the | ||
| /// strings in the dialog. | ||
| /// </param> | ||
| protected void LaunchSimpleEffectDialog (AddinLocalizer localizer) |
There was a problem hiding this comment.
I'm pretty sure this signature (or some equivalent way of doing this) is still needed for addins, since this wrapper translates the Mono.Addins.AddinLocalizer into the IAddinLocalizer interface
Addins can't create an AddinLocalizerWrapper manually either since that's an internal class.
e.g. https://github.com/PintaProject/NightVisionEffect/blob/a9ac90a38373cec4c5f69d042998c4ffe35e6438/NightVisionAddin/NightVisionEffect.cs#L53
and
168115e
There was a problem hiding this comment.
Thank you. I've re-added it, but the return type is a Task<bool>. Is that fine?
There was a problem hiding this comment.
Yeah changing the return type to Task<bool> is fine - all the demo add-ins will need to be updated for any API changes before release
There was a problem hiding this comment.
@cameronwhite I know that you are referring to API changes and demos in general (in all the extensible parts of Pinta). With that being said, is there another demo effect besides the night vision one?
There was a problem hiding this comment.
No, I think that's the only demo for an effect
| @@ -84,35 +84,12 @@ public abstract class BaseEffect | |||
| /// Launches the configuration dialog for this effect/adjustment. | |||
| /// If IsConfigurable is true, the ConfigDialogResponse event will be invoked when the user accepts or cancels the dialog. | |||
| /// </summary> | |||
| public virtual void LaunchConfiguration () | |||
There was a problem hiding this comment.
What do you think about making the return type a Task<bool>, or some wrapper around a bool to make the meaning more clear, sort of like the ConfigDialogResponseEventArgs from before?
My main thoughts are
Gtk.ResponseTypehas a lot of values in the enum, so it's less clear which values we care about or are possible to have to deal with. It's really just success / failure that matter- Ideally the effect API should be as independent as possible from UI stuff like GTK. Using the
ResponseTypeenum probably doesn't cause anything bad like forcing GTK libraries to be loaded / initialized for the unit tests, but it does mean that any API changes when upgrading to future GTK versions have more of an impact on the effects
There was a problem hiding this comment.
I think Task<bool> is good for now while the API is being defined. I just changed it
This reverts commit 130c569.
…ype>` to `Task<bool>`
…MessageDialogHandler`
…Lehonti/Pinta_fork into refactoring/effect_config_async
|
I addressed your points @cameronwhite I was about to change the signature of other methods that show a dialog, too, but it got the compiler to show a lot of warnings, so I reverted those commits and decided to address those issues in some future pull request. |
|
Thanks, yeah I think limiting this PR to the effect dialog changes makes sense. |
Refactoring
AsyncEffectRendererproperly is currently hard because it's hard to track when exactly things are happening.For instance, methods where configuration dialogs are created return
voidand rely on event handlers for the dialog's asynchronous tasks, most importantly what happens when the user clicks eitherOkorCancel. This leaves these dialogs 'hanging'.The above is what I'm addressing in this pull request, but there are many other things that I plan to address in future pull requests.